Refactor: Standardize PackageManager #1538
Conversation
edvilme
changed the title
PackageManager
edvilme
force-pushed
the
vscode-python-environments-package-refactor
branch
2 times, most recently
from
ce7e024 to
fc73d95
Compare
|
These refactorings make adding new features to the Package Managers (e.g., #1530) much easier by properly delegating responsibilities and using clear API names. |
|
This one I will need to consider more as they adjust the api surface. I would be more comfortable with a change where we instead added an optional arg to the get or manage packages that allowed you to specify if it should refresh cache. So we have the same surface generally just allows someone to specify what type of change it should be as it relates to the cache. What are your thoughts on a design like this? |
edvilme
force-pushed
the
vscode-python-environments-package-refactor
branch
from
9d57c50 to
986e84a
Compare
edvilme
force-pushed
the
vscode-python-environments-package-refactor
branch
from
3eefeef to
03cd8f0
Compare
| * When `true`, bypasses the cache and fetches the latest packages from the underlying tool. | ||
| * Defaults to `false`. | ||
| */ | ||
| skipCache?: boolean; |
There was a problem hiding this comment.
is the point of going with options that then has the skipCache inside is just more flexibility down the line? Did you have anything else in mind you were going to put into this object?
There was a problem hiding this comment.
Yes, for flexibility down the line. Perhaps adding an option for overriding the feed, or perhaps specifying some filter
|
just the one question but overall looks good!! |
| await this.refresh(environment); | ||
| async getPackages(environment: PythonEnvironment, options?: GetPackagesOptions): Promise<Package[] | undefined> { | ||
| if (options?.skipCache || !this.packages.has(environment.envId.id)) { | ||
| const packages = await this.fetchPackagesFromTool(environment); |
There was a problem hiding this comment.
Should this be handled more elegantly? The old getPackage calls refresh() that shows a friendly error not blocking UI, this one will just throw an uncaught exception.
| if (!this.packages.has(environment.envId.id)) { | ||
| await this.refresh(environment); | ||
| async getPackages(environment: PythonEnvironment, options?: GetPackagesOptions): Promise<Package[] | undefined> { | ||
| if (options?.skipCache || !this.packages.has(environment.envId.id)) { |
There was a problem hiding this comment.
all old getPackages are wrapped in withProgress in refresh, any reason why they are got rid of now?
There was a problem hiding this comment.
Flow goes refresh() -> updatePackagesAndNotify() -> getPackages()
Refresh handles the withProgress and exception catching
updatePackagesAndNotify centralizes the updating (also makes marking packages as (in)transitive easier
This pull request introduces a new
GetPackagesOptionsinterface to support additional options (such as cache bypassing) when retrieving Python packages from a given environment. It updates the relevant APIs, interfaces, and implementations to accept this new options parameter, and refactors the pip package manager logic to support cache skipping and code simplification. The changes are applied consistently across the main codebase and sample/example files.API and Interface Enhancements:
Added a new
GetPackagesOptionsinterface, allowing callers to specify options (currentlyskipCache) when retrieving packages. This is now accepted bygetPackagesmethods inPackageManagerandPythonPackageGetterApiinterfaces. [1] [2] [3] [4] [5] [6] [7] [8]Updated all usages and implementations of
getPackagesto accept the new optionaloptionsparameter, including inPythonEnvironmentApiImpl,InternalPackageManager, and related example/sample APIs. [1] [2] [3] [4]Pip Package Manager Refactoring and Improvements:
Renamed
pipManager.tstopipPackageManager.tsand updated imports accordingly.Refactored the pip package manager to support cache bypassing: if
options.skipCacheis set, it fetches a fresh package list instead of using the cache. Also simplified the logic for managing and refreshing packages, delegating change notification to a new shared utility. [1] [2] [3]Removed redundant code for change detection and replaced with a shared
updatePackagesAndNotifyutility. [1] [2] [3]Utilities and Supporting Changes:
execPipList) that supports additional arguments, and removed now-unnecessary helper functions. [1] [2] [3] [4]Code Style and Minor Cleanups:
These changes improve the flexibility and maintainability of the package management APIs and their implementations, preparing the codebase for future enhancements and better cache control.